-
Notifications
You must be signed in to change notification settings - Fork 37
Implement remaining tagless uints #993
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
… and prepare for safe e-group writer
Codecov Report❌ Patch coverage is ❌ Your patch status has failed because the patch coverage (74.64%) is below the target coverage (85.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #993 +/- ##
==========================================
- Coverage 78.28% 78.28% -0.01%
==========================================
Files 138 138
Lines 34036 34093 +57
Branches 34036 34093 +57
==========================================
+ Hits 26644 26688 +44
- Misses 5329 5334 +5
- Partials 2063 2071 +8 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Build & Test fails due to clippy.. looks like a new rust release may have adjusted clippy lints. |
ParameterEncoding::UInt16 => { | ||
let (fixed_uint_lazy_value, remaining) = try_or_some_err! { | ||
self.remaining_args_buffer.read_fixed_uint_as_lazy_value(2) | ||
}; | ||
let value_ref = &*self | ||
.remaining_args_buffer | ||
.context() | ||
.allocator() | ||
.alloc_with(|| fixed_uint_lazy_value); | ||
( | ||
EExpArg::new(parameter, EExpArgExpr::ValueLiteral(value_ref)), | ||
remaining, | ||
) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a lot of duplication here that could be cleaned up with something like this, if you change the argument type for read_fixed_uint_as_lazy_value
and inverted the match
statement in that function.
ParameterEncoding::UInt8,
ParameterEncoding::UInt16,
ParameterEncoding::UInt32,
ParameterEncoding::UInt64, => {
let (fixed_uint_lazy_value, remaining) = try_or_some_err! {
self.remaining_args_buffer.read_fixed_uint_as_lazy_value(parameter.encoding())
};
let value_ref = &*self
.remaining_args_buffer
.context()
.allocator()
.alloc_with(|| fixed_uint_lazy_value);
(
EExpArg::new(parameter, EExpArgExpr::ValueLiteral(value_ref)),
remaining,
)
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good call. I implemented it slightly different though. I didn't want to bring the ParamaterEncoding
into the raw binary reader (where read_fixed_uint_as_lazy_value
lives) since it would leak a higher level template type into the low level reader, so I added a TryFrom
impl to convert ParameterEncoding
into BinaryValueEncoding
, which is a nearly mirrored enum (minus macro shape). The rest is just as your suggestion.
Added a new commit to address the clippy warnings, not sure if that's why the merge button got disabled or not. |
Issue #, if available: #943
Description of changes:
Prior to this PR (as of #991) supported tagless encoding was limited to
uint8
. This PR extends the unsigned int support to the remaininguint
types by implementinguint16
,uint32
anduint64
.CLI Testing
Below is an inspect of an encoding and usage of the following macro:
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.